fix: send customData with multi-recipient SMS sends - #16
Open
eastagiletracker wants to merge 1 commit into
Open
Conversation
SMSService.SendAsync built the outgoing SMSCampaign from Accounts, Message, Title and SenderPhone only, so SMSRequest.CustomData never reached the wire. Callers of the multi-recipient overload lost the value silently; only SendSingleAsync worked, because CreateSingle copies customData onto the single Account it builds, where it serializes as messageData. Apply a campaign-level CustomData to every account that does not define its own. Per-account values keep precedence and the caller's Account instances are left untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes fixing the
customDataargument on the multi-recipientSMS.SendAsyncoverload, which is documented as sent with the message but never reaches the API. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/504. You can sign in with your GitHub ID to claim ownership of the project.What is wrong
ISMSService.SendAsync(accounts, message, title, customData: ...)documentscustomDataas "Custom data to be included with the message", but the value is discarded before the request is built.SMSService.SendAsync(SMSRequest)constructs the outgoingSMSCampaignfromAccounts,Message,TitleandSenderPhoneonly, soSMSRequest.CustomDatahas nowhere to go — the API carries custom data per recipient, asAccount.CustomDataserialized tomessageData.SendSingleAsyncis unaffected becauseSMSRequest.CreateSinglecopiescustomDataonto the singleAccountit builds; only the multi-recipient path loses it, silently and with no error.Reproducing it on
main(c8c7bc3)A ~30-line console program that references
src/CCAI.NETand swaps in anHttpMessageHandlerthat captures the outgoing body instead of calling the API:The captured request body on
maintoday —ORD-123is nowhere in it:{ "accounts": [ { "firstName": "John", "lastName": "Doe", "phone": "+15551234567" }, { "firstName": "Jane", "lastName": "Doe", "phone": "+15557654321" } ], "message": "Hello ${FirstName}!", "title": "Order confirmations" }With this change, the same program sends:
{ "accounts": [ { "firstName": "John", "lastName": "Doe", "phone": "+15551234567", "messageData": "{\"orderId\":\"ORD-123\"}" }, { "firstName": "Jane", "lastName": "Doe", "phone": "+15557654321", "messageData": "{\"orderId\":\"ORD-123\"}" } ], "message": "Hello ${FirstName}!", "title": "Order confirmations" }The change
Eleven lines in
SMSService.SendAsync: whenrequest.CustomDatais set, it is applied to every account that does not already define its own, immediately before theSMSCampaignis built. This matches howccai-nodemapscustomDataontomessageDatafor each recipient in its ownsend().It is deliberately conservative. A per-account
CustomDataalways wins, so a caller who already sets it per recipient sees no change.Accountis a record and the accounts are projected withwith, so the caller's own instances are never mutated. No public signature changes, and since the value was previously dropped outright, no existing caller can be depending on the old behaviour.How it was verified
Four tests added to
tests/CCAI.NET.Tests/SMS/SMSCustomDataWebhookTests.cs: two recipients both receiving a request-levelcustomData; a per-account value taking precedence over it; the null case, where nothing is added; and a check that the caller'sAccountinstances come back untouched. The first two fail on the unmodified tree (Expected: "OrderBatch-42" / Actual: null) and pass with the fix.dotnet build CCAI.NET.slnanddotnet test CCAI.NET.slnwere run on the clean tree first as a baseline and again afterwards. Baseline: build succeeded, 121 warnings (all pre-existingCS1591/CS1573doc warnings), 75 tests passed, none failed. After: build succeeded, the same 121 warnings, 79 tests passed, none failed. No new failures and no new warnings.One thing worth flagging: your open #15 edits the argument validation a few lines above this hunk on the
testbranch. The two changes sit in the same method but not on the same lines, so I would expect them to apply cleanly in either order.How this was managed
This work was tracked as a single story, SMS request-level customData is dropped for multi-recipient sends, on a board at https://eastagiletracker.com/projects/504 that was imported from this repository's own issues and pull requests — 15 stories, mirroring your merged and open work into iterations.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com